RSACng.Encrypt used wrong "Pkcs1" link. #2849

CXuesong · 2019-07-27T07:23:12Z

Summary

padding used in RSACng.Encrypt accepts a RSAEncryptionPadding instance, but exception description for CryptographicException used link to RSASignaturePadding. RSACng.Encrypt from RefSrc makes me thinking this exception description should be the same as the one from RSACng.Decrypt.

mairaw

Thank you for noticing and fixing the exception issue @CXuesong. I left a suggestion for you to consider before we merge this.

mairaw · 2019-07-28T02:37:24Z

xml/System.Security.Cryptography/RSACng.xml


 -or-  

 <paramref name="padding" /> is <see langword="null" />.</exception>
        <exception cref="T:System.Security.Cryptography.CryptographicException">
-          <paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSASignaturePadding.Pkcs1" /> or <see cref="P:System.Security.Cryptography.RSASignaturePadding.Pss" />.</exception>
+          <paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>


Suggested change

<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>

<paramref name="padding" />.<see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> isn't equal to <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" /> or <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>

I think that's the wrong "or", Maira. The exception is thrown if the padding mode isn't (pkcs1 or oaep), the only two supported modes. I read the "-or-" version as it throws always (throws if != pkcs1, and throws if != oaep... nothing is both, so throws always)

I have copied this statement from the same documentation from the Decrypt method, and I think that one should work.

dotnet-api-docs/xml/System.Security.Cryptography/RSACng.xml

Lines 220 to 230 in 056a526

<returns>The decrypted data.</returns>

<remarks>To be added.</remarks>

<exception cref="T:System.ArgumentNullException">

<paramref name="data" /> is <see langword="null" />.

-or-

<paramref name="padding" /> is <see langword="null" />.</exception>

<exception cref="T:System.Security.Cryptography.CryptographicException">

<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>

</Docs>

Ok, I'll reword my suggestion. If both conditions should be met, I think we should use and then. Perhaps we might also need to review the Decrypt exception condition @CXuesong.

@bartonjs can you review my suggestion?

<paramref name="padding" />.<see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> isn't equal to <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" /> or <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception> looks reasonable to me.

Except that P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1 should probably be F:System.Security.Cryptography.RSAEncryptionPaddingMode.Pkcs1

carlossanlop · 2019-07-28T03:19:53Z

@bartonjs is actively documenting these files too. Mentioning him here so he takes a look before merging, @mairaw.

CXuesong · 2019-08-08T14:34:16Z

Is there any change I need to make before this PR can be accepted? After looking at @mairaw 's & @bartonjs 's comments, I think I may keep current version without changing into "-- or --" version.

mairaw · 2019-08-08T23:00:58Z

Checking latest comments now...

CXuesong · 2019-08-09T14:44:23Z

Done modification. In this case, may I also change the exception description in RSACng.Decrypt to keep them consistent?

dotnet-api-docs/xml/System.Security.Cryptography/RSACng.xml

Lines 228 to 230 in 056a526

    
             <exception cref="T:System.Security.Cryptography.CryptographicException"> 
        
               <paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception> 
        
           </Docs>

carlossanlop · 2019-12-12T16:58:32Z

@CXuesong would you mind rebasing your branch to the latest content in master? @bartonjs has been adding a lot of improvements to the Cryptography API documentation.

@bartonjs would you mind answering @CXuesong 's question?

bartonjs · 2019-12-12T17:02:52Z

Well, my update of RSA (including this file) is currently in PR, so rebasing should be put off a tiny bit (if I have to make another round of updates I can just take this into mine).

And, yeah, making Decrypt match Encrypt seems good 😄

bartonjs · 2019-12-12T21:41:22Z

My updates to the RSA classes went in, so rebasing this and incorporating the change for Decrypt are now actionable.

BillWagner · 2020-03-02T20:09:27Z

ping @carlossanlop @bartonjs

Can one of you help me get the correct status of this one? What is the next action?

stale review

mairaw assigned CXuesong Jul 28, 2019

mairaw added this to the July 2019 milestone Jul 28, 2019

mairaw previously requested changes Jul 28, 2019

View reviewed changes

mairaw added the ✨ 1st-time dotnet-api-docs contributor! Indicates PRs from new contributors to the dotnet-api-docs repository label Jul 28, 2019

bartonjs approved these changes Jul 28, 2019

View reviewed changes

mairaw added the waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged label Aug 8, 2019

CXuesong added 3 commits December 14, 2019 00:10

RSACng.Encrypt used wrong "Pkcs1" link.

d5d0f04

Checked the source code. Corrrection.

35173fc

Resolve comment.

1393cbf

CXuesong force-pushed the master branch from d4c8954 to 1393cbf Compare December 13, 2019 16:34

Keep exception description in Decrypt consistent with Encrypt.

d1a0571

BillWagner modified the milestones: July 2019, March 2020 Mar 2, 2020

bartonjs approved these changes Mar 2, 2020

View reviewed changes

carlossanlop merged commit 9a9a22f into dotnet:master Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RSACng.Encrypt used wrong "Pkcs1" link. #2849

RSACng.Encrypt used wrong "Pkcs1" link. #2849

CXuesong commented Jul 27, 2019

Uh oh!

mairaw left a comment

Uh oh!

mairaw Jul 28, 2019 •

edited

Loading

Uh oh!

bartonjs Jul 28, 2019

Uh oh!

CXuesong Jul 28, 2019

Uh oh!

mairaw Aug 8, 2019

Uh oh!

mairaw Aug 8, 2019

Uh oh!

bartonjs Aug 9, 2019

Uh oh!

carlossanlop commented Jul 28, 2019

Uh oh!

CXuesong commented Aug 8, 2019

Uh oh!

mairaw commented Aug 8, 2019

Uh oh!

CXuesong commented Aug 9, 2019

Uh oh!

carlossanlop commented Dec 12, 2019

Uh oh!

bartonjs commented Dec 12, 2019

Uh oh!

bartonjs commented Dec 12, 2019

Uh oh!

BillWagner commented Mar 2, 2020

Uh oh!

Uh oh!

	<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>
	<paramref name="padding" />.<see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> isn't equal to <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" /> or <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>

	<returns>The decrypted data.</returns>
	<remarks>To be added.</remarks>
	<exception cref="T:System.ArgumentNullException">
	<paramref name="data" /> is <see langword="null" />.

	-or-

	<paramref name="padding" /> is <see langword="null" />.</exception>
	<exception cref="T:System.Security.Cryptography.CryptographicException">
	<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>
	</Docs>

RSACng.Encrypt used wrong "Pkcs1" link. #2849

RSACng.Encrypt used wrong "Pkcs1" link. #2849

Conversation

CXuesong commented Jul 27, 2019

Summary

Uh oh!

mairaw left a comment

Choose a reason for hiding this comment

Uh oh!

mairaw Jul 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs Jul 28, 2019

Choose a reason for hiding this comment

Uh oh!

CXuesong Jul 28, 2019

Choose a reason for hiding this comment

Uh oh!

mairaw Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

mairaw Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

bartonjs Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

carlossanlop commented Jul 28, 2019

Uh oh!

CXuesong commented Aug 8, 2019

Uh oh!

mairaw commented Aug 8, 2019

Uh oh!

CXuesong commented Aug 9, 2019

Uh oh!

carlossanlop commented Dec 12, 2019

Uh oh!

bartonjs commented Dec 12, 2019

Uh oh!

bartonjs commented Dec 12, 2019

Uh oh!

BillWagner commented Mar 2, 2020

Uh oh!

Uh oh!

mairaw Jul 28, 2019 •

edited

Loading